Add SDK MCP OAuth host token handlers#1669
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 2.1M
| export interface McpAuthToken { | ||
| /** Access token acquired by the SDK host. */ | ||
| accessToken: string; | ||
| /** OAuth token type. Defaults to Bearer when omitted. */ | ||
| tokenType?: string; | ||
| /** Refresh token supplied by the host, if available. */ | ||
| refreshToken?: string; | ||
| /** Token lifetime in seconds, if known. */ | ||
| expiresIn?: number; | ||
| } |
There was a problem hiding this comment.
what about an idToken? idTokens are useful as they contain claims that are good to display like usernames/email/etc.
There was a problem hiding this comment.
So unless I'm mistaken, the runtime wouldn't have any actual use for it; all it needs as a response from the host is the access token to be forwarded back to the MCP server for access... In fact, I'm about to remove the refreshToken from here, since as discussed in yesterday's meeting, refresh would be the host's responsibility, and so the runtime has no use for that.
But let me know if I have the wrong mental model here! Plus we can always add it later.
59694fd to
6d3d655
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 1.8M
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 3.1M
a4cfe07 to
e3aae80
Compare
This comment has been minimized.
This comment has been minimized.
530ce4b to
a16180e
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 2.7M
1ef3726 to
87f1785
Compare
This comment has been minimized.
This comment has been minimized.
51c9235 to
d5c709b
Compare
Expose host-delegated MCP OAuth handling across SDK languages, sync generated RPC and event models to the lifecycle contract, and add cross-language E2E coverage for initial auth, refresh, upscope, reauth, and cancellation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d5c709b to
70ec1fd
Compare
Cross-SDK Consistency ReviewThis PR successfully threads MCP OAuth host token handler support through all six SDK implementations. The wire-format types and event-interest registration logic are well-aligned across the board. One consistency finding is worth addressing before merge.
|
| Handler | Signature |
|---|---|
PermissionHandler |
handle(PermissionRequest request, PermissionInvocation invocation) |
UserInputHandler |
handle(UserInputRequest request, UserInputInvocation invocation) |
ExitPlanModeHandler |
handle(ExitPlanModeRequest request, ExitPlanModeInvocation invocation) |
This two-parameter pattern (request + invocation) also matches Node.js, Python, and Go:
- Node.js:
(request: McpAuthRequest, context: { sessionId: string }) - Python:
(request: McpAuthRequest, context: McpAuthContext) - Go:
(request MCPAuthRequest, invocation MCPAuthInvocation)
The new McpAuthHandler instead uses a single-parameter signature with sessionId embedded directly in McpAuthRequest:
// Current (inconsistent)
public record McpAuthRequest(String sessionId, String requestId, String serverName, ...)
CompletableFuture<McpAuthResult> handle(McpAuthRequest request);This is unusual because sessionId is SDK-layer context, not a field from the MCP server — it doesn't belong in a type called McpAuthRequest.
Suggested fix — follow the established Java SDK pattern:
// Add a companion invocation class
public final class McpAuthInvocation {
private String sessionId;
// getter/setter
}
// Remove sessionId from McpAuthRequest record
public record McpAuthRequest(String requestId, String serverName, String serverUrl,
McpOauthRequestReason reason, ...)
// Update handler signature
CompletableFuture<McpAuthResult> handle(McpAuthRequest request, McpAuthInvocation invocation);Alternatively, following the ElicitationHandler pattern (single merged context class), McpAuthRequest could be renamed to McpAuthContext — though that would make it the only context-style handler that bundles requestId, which is slightly unusual.
✅ Everything else is consistent
- .NET uses
McpAuthContext(merging all fields into a single context) — consistent with .NET's ownElicitationContextpattern. - Rust exposes
session_idandrequest_idas strongly-typed separate parameters — consistent with Rust's ownPermissionHandlerapproach. - All six SDKs register
mcp.oauth_requiredevent interest only when the handler is configured. - Wire format types (
reason,wwwAuthenticateParams,resourceMetadata,staticClientConfig,accessToken,tokenType,expiresIn) are structurally equivalent across all SDKs.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 2.3M · ◷
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 2.3M
| * | ||
| * @since 1.0.0 | ||
| */ | ||
| public record McpAuthRequest(String sessionId, String requestId, String serverName, String serverUrl, |
There was a problem hiding this comment.
Including sessionId in McpAuthRequest mixes SDK-layer session context with server-side request data, which is inconsistent with the established Java SDK pattern.\n\nEvery other Java handler in this SDK separates these concerns via a companion XxxInvocation type:\n- PermissionHandler: handle(PermissionRequest request, PermissionInvocation invocation)\n- UserInputHandler: handle(UserInputRequest request, UserInputInvocation invocation)\n- ExitPlanModeHandler: handle(ExitPlanModeRequest request, ExitPlanModeInvocation invocation)\n\nThis two-parameter pattern (request + invocation) also matches Node.js, Python, and Go, where sessionId is always in a separate context/invocation argument rather than bundled into the request object.\n\nSuggested: add a McpAuthInvocation class with sessionId (following PermissionInvocation), remove sessionId from this record, and update McpAuthHandler.handle to handle(McpAuthRequest request, McpAuthInvocation invocation).
| * the MCP OAuth request context | ||
| * @return a future resolving to token data or cancellation | ||
| */ | ||
| CompletableFuture<McpAuthResult> handle(McpAuthRequest request); |
There was a problem hiding this comment.
This single-parameter signature deviates from the two-parameter pattern used by all other Java handlers in the SDK:\n\njava\n// All other Java handlers:\nCompletableFuture<PermissionRequestResult> handle(PermissionRequest request, PermissionInvocation invocation);\nCompletableFuture<UserInputResponse> handle(UserInputRequest request, UserInputInvocation invocation);\nCompletableFuture<ExitPlanModeResult> handle(ExitPlanModeRequest request, ExitPlanModeInvocation invocation);\n\n\nNode.js, Python, and Go also pass session context as a second argument (separate from the request). Consider updating this to:\n\njava\nCompletableFuture<McpAuthResult> handle(McpAuthRequest request, McpAuthInvocation invocation);\n\n\nwith McpAuthInvocation holding sessionId and McpAuthRequest containing only the MCP-server-originated fields (requestId, serverName, serverUrl, reason, etc.).
Summary
reason, optionalwwwAuthenticateParams.resourceMetadataUrl, optional rawresourceMetadata, and no hostrefreshTokenin token responses.mcp.oauth_requiredinterest only when the high-level MCP auth callback is configured.scope,resourceMetadataUrl,error=insufficient_scope, and refresherror=invalid_token).Notes
Use local runtime for MCP OAuth E2E validation) points E2E harnesses at/Users/roji/.copilot/repos/copilot-worktrees/copilot-agent-runtime/roji-symmetrical-dollop/dist-cli/index.jsfor runtime PR #11277 validation. This should be removed before final merge.initial -> refresh -> upscope -> refresh-cancel -> reauthacross all supported SDK languages.session.mcp.reloadWithConfigworkaround was removed; Node E2E passes without it against the current runtime branch.Validation
cd test/harness && npm cicd nodejs && npm run typecheck && npx vitest run test/e2e/mcp_oauth.e2e.test.tscd python && uv run pytest e2e/test_mcp_oauth_e2e.pycd go && go test ./internal/e2e -run TestMCPOAuthE2E -count=1cd dotnet && DOTNET_ROLL_FORWARD=Major dotnet test test/GitHub.Copilot.SDK.Test.csproj --filter FullyQualifiedName~McpOAuthE2ETestscd rust && export PATH="$HOME/.cargo/bin:$PATH" && cargo test --features test-support --test e2e mcp_oauth -- --nocapturecd java && mvn test -Dtest=McpOAuthE2ETestgofmt -w internal/e2e/mcp_oauth_e2e_test.go,cargo fmt -- tests/e2e/mcp_oauth.rs, andmvn spotless:apply -Dspotless.check.skip=false.